Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infra: Migrate to gradle #783

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Infra: Migrate to gradle #783

wants to merge 18 commits into from

Conversation

germanosin
Copy link
Member

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • [] Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

@germanosin germanosin requested review from a team as code owners January 15, 2025 11:45
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Jan 15, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi germanosin! 👋

Welcome, and thank you for opening your first PR in the repo!

Please wait for triaging by our maintainers.

Please take a look at our contributing guide.

api/build.gradle Outdated

dependencies {
implementation project(":contract")
implementation project(":frontend")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api shouldn't depend on the frontend

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On prod build should

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm not quite, in maven there's a skipUIBuild flag to skip UI build even in prod profile. Let's address this functionality

api/build.gradle Outdated

checkstyle {
toolVersion '10.3.1'
configFile = rootProject.file('etc/checkstyle/checkstyle-e2e.xml')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
configFile = rootProject.file('etc/checkstyle/checkstyle-e2e.xml')
configFile = rootProject.file('etc/checkstyle/checkstyle.xml')

due to e2e having a different rule set


plugins {
id "java-library"
id 'org.openapi.generator' version '7.9.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we extract this version as well to a version catalog (gradle's "dependency management" list) to make it possible for dependabot to track these versions as well?


checkstyle {
toolVersion '10.3.1'
configFile = rootProject.file('etc/checkstyle/checkstyle.xml')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
configFile = rootProject.file('etc/checkstyle/checkstyle.xml')
configFile = rootProject.file('etc/checkstyle/checkstyle-e2e.xml')

@@ -0,0 +1,7 @@
description='Kafka UI'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description='Kafka UI'
description='Kafbat UI'

gradle/libs.versions.toml Show resolved Hide resolved
api/build.gradle Outdated
plugins {
id 'antlr'
id 'checkstyle'
id 'org.springframework.boot' version '3.4.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gradle/libs.versions.toml has 3.3.6

@@ -0,0 +1,103 @@
[versions]
spring-boot = '3.3.6'
spring-ldap = '6.3.5'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 6.3.2? in maven we don't override this and the current version is 6.4.2

// repositories {
// sonatype {
// nexusUrl = uri("https://s01.oss.sonatype.org/service/local/")
// snapshotRepositoryUrl = uri("https://s01.oss.sonatype.org/content/repositories/snapshots/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? 👀

@@ -0,0 +1,2 @@
sonatypeUsername=test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to commit these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need any value to be present during build time

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can any value be ""? test is a bit confusing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's a bit confusing

@Haarolean Haarolean requested a review from yeikel January 15, 2025 12:19
@Haarolean Haarolean added scope/backend Related to backend changes and removed status/triage/manual Manual triage in progress labels Jan 15, 2025
@Haarolean Haarolean linked an issue Jan 15, 2025 that may be closed by this pull request
@Haarolean Haarolean added the scope/infra CI, CD, dev. env, etc. label Jan 15, 2025
@Haarolean Haarolean changed the title Gradle Infra: Migrate to gradle Jan 15, 2025
}

checkstyle {
toolVersion '10.3.1'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be added to the catalog?

Comment on lines +8 to +10
kafkaVersion = "3.8.0"
aspectjVersion = "1.9.21"
allureVersion = "2.27.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for these

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this version does not seem correct. It should be from the confluent distribution

Comment on lines +17 to +19
implementation "commons-io:commons-io:2.16.1"
implementation "org.testng:testng:7.10.0"
implementation "com.codeborne:selenide:7.2.3"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please add this to the catalog as well

}

checkstyle {
toolVersion '10.3.1'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version should ideally be shared with the other module


implementation libs.aws.msk.auth
implementation (libs.azure.identity) {
exclude group: 'io.netty', module: 'netty-tcnative-boringssl-static'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please explain why we are excluding this

@yeikel
Copy link
Collaborator

yeikel commented Jan 15, 2025

Please update the dependabot configuration as well 🙏

See

- package-ecosystem: maven

Comment on lines +52 to +54
artifact(javadocJar) {
classifier = 'javadoc'
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? Do we publish our JAR to central, or who consumes us as JAR directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serde-api is quite the name

Comment on lines +120 to +122
from {
image = 'azul/zulu-openjdk-alpine:21.0.5-jre-headless'
}
Copy link
Collaborator

@yeikel yeikel Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be delegated to a Dockerfile or read from the existing Dockerfile?

The main downside of setting this here is that dependabot won't get any visibility and we will need to update it manually

Copy link
Member

@Haarolean Haarolean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, in maven we have prod and local profiles, the latter aimed at skipping frontend build. Can we address this as well please?


jib {
from {
image = 'azul/zulu-openjdk-alpine:21.0.5-jre-headless'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a few major issues with using jib:

  1. Currently, in our Dockerfile we have a few additional packages installed:
  • gcompat required for snappy
  • tzdata as requested by someone in the past to be able to configure timezones properly in the container
  1. We're creating a /etc/kafkaui/ dir for a dynamic config. I am unsure what will happen with the current implementation, dynamic config operations might fail.
  2. We're running the app as a non-root user there, not sure what's the default behavior with jib?
  3. Currently dependabot can properly bump the base image version present in dockerfile, however, it won't be able to work with this version located here, in api/build.gradle.

Overall, I prefer dockerfile build with a mvn/gradle plugin rather than jib in favor of more granular customization possibilities.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus the lack of dependabot support mentioned above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/backend Related to backend changes scope/infra CI, CD, dev. env, etc. status/triage/completed Automatic triage completed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Gradle
4 participants